Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix regression where some CLI commands break on Ruby 1.8 #7738

Merged
merged 1 commit into from
Dec 2, 2014
Merged

Fix regression where some CLI commands break on Ruby 1.8 #7738

merged 1 commit into from
Dec 2, 2014

Conversation

claui
Copy link
Contributor

@claui claui commented Dec 2, 2014

This fixes a regression where for example, brew cask list would break on Ruby 1.8.7.

Cause

I feel the regression might have been introduced in the merge commit 62c1ce5, which adds code to path_base.rb which depends on Gem::Version, but doesn’t require the rubygems gem.

A Google search showed that rubygems is built into 1.9 and later but not into 1.8 (I learn something new every day!). This seems to cause brew cask list (and other commands) to break under Ruby 1.8 since 62c1ce5.

Steps to reproduce

Assuming rbenv and Ruby 1.8.7-p375 are installed, follow these steps:

  1. Make sure homebrew-cask is in development mode.
  2. In case you don’t have 1.8.7-p375 installed, change the command below to the patch level you have.
  3. Run brew cask list under Ruby 1.8 like so:
RBENV_VERSION=1.8.7-p375 HOMEBREW_BREW_FILE=/usr/local/bin/brew ruby /usr/local/Library/brew.rb /usr/local/bin/brew-cask list

Result

Error: uninitialized constant Cask::Source::PathBase::Gem while loading '/usr/local/Library/Taps/caskroom/homebrew-cask/Casks/clarify.rb'
Please report this bug:
    https://github.com/caskroom/homebrew-cask/issues
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/source/path_base.rb:51:in `load'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask.rb:78:in `load'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/scopes.rb:87:in `installed'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/scopes.rb:81:in `map'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/scopes.rb:81:in `installed'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli/list.rb:52:in `list_installed'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli/list.rb:10:in `run'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli.rb:81:in `run_command'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli.rb:121:in `process'
/usr/local/bin/brew-cask.rb:42
/usr/local/Library/brew.rb:59:in `require'
/usr/local/Library/brew.rb:59:in `require?'
/usr/local/Library/brew.rb:118

CLI::Alfred

  • There appears to be a similar constellation in the CLI::Alfred class (since 0f664ca). That commit has been around since 0.41.1 so I guess it doesn’t cause any issues after all. I couldn’t produce any problem with this either but I added the missing require here as well just in case.

Test coverage

Given that brew cask list breaks, I would have expected list_test.rb to break as well. However, for some strange reason all the unit tests are still green:

cd "$(brew --repository)"/Library/Taps/caskroom/homebrew-cask/lib/..

RBENV_VERSION=1.8.7-p375 bundle exec rake test TEST=test/cask/cli/list_test.rb

Strangely, the result is:

...

Finished in 7.214952s, 0.4158 runs/s, 0.4158 assertions/s.

3 runs, 3 assertions, 0 failures, 0 errors, 0 skips

Please review

Pinging one of @federicobond @ndr-qef @phinze @rolandwalker to please have a look. 😊

Thank you!

@claui claui added core Issue with Homebrew itself rather than with a specific cask. and removed core Issue with Homebrew itself rather than with a specific cask. labels Dec 2, 2014
@phinze
Copy link
Contributor

phinze commented Dec 2, 2014

First of all, hello and welcome @claui 👋 😀

And second - wow great work researching and documenting this issue!

However, for some strange reason all the unit tests are still green

I suspect this is the case because 'rubygems' ends up getting required somewhere in the mountain of setup that happens in test_helper.rb. I think the effort to arrange a test scenario that would cover probably outweighs the value it would provide, so I'm okay with this just being documented here and in the commit message.

As a nitpick - I generally lean towards a bit more verbosity in the commit message. There's great documentation work done here in the PR, but it would be more difficult for our Future Selves running git blame and reading the commit message for the require 'rubygems' line to answer the question "why does this fix it?". But like a say - just a nitpick. 🐒

So 👍 from me - happy to have you on the team!

@claui
Copy link
Contributor Author

claui commented Dec 2, 2014

Thanks @phinze! 😄

By the way, I just found #7529 and #7530 where requiring (or not requiring) rubygems has already been discussed. Does this imply that the plan is to lose the dependency on rubygems altogether?

@rolandwalker
Copy link
Contributor

Those PRs are just me not realizing that rubygems was being used for more than the transitional Tap migration code. It also turns out I need Gem::Version for #7696.

@claui
Copy link
Contributor Author

claui commented Dec 2, 2014

Thank you @rolandwalker for clarifying this.
So would you prefer me to move that require back to the central location where it used to be?

@phinze
Copy link
Contributor

phinze commented Dec 2, 2014

Does this imply that the plan is to lose the dependency on rubygems altogether?

Yeah we will probably never drop the dependency on rubygems - we're mostly just focused on killing the dependency on homebrew code (see #5080).

FWIW I think keeping the requires closer to the Gem::Version usages is nicer, so I'm +1 for merge as-is. Will let @rolandwalker comment tho.

@rolandwalker
Copy link
Contributor

+1 for merge as-is. I did the same in lib/cask/installer.rb (#7696), which merged while this was pending.

rolandwalker added a commit that referenced this pull request Dec 2, 2014
@rolandwalker
Merge pull request #7738 from claui/fix-brew-cask-list-ruby18
1c5c6ae 
Fix regression where some CLI commands break on Ruby 1.8
@rolandwalker rolandwalker merged commit 1c5c6ae into Homebrew:master Dec 2, 2014
@claui claui deleted the fix-brew-cask-list-ruby18 branch December 2, 2014 18:07
@miccal miccal removed the core Issue with Homebrew itself rather than with a specific cask. label Dec 23, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@claui @phinze @rolandwalker @miccal